Skip to content

Conversation

@kr-2003
Copy link
Contributor

@kr-2003 kr-2003 commented May 21, 2025

EDIT by Anutosh : For anyone interested in this change. Serge and I had introduced a hacky patch to build llvm back in Dec 2023. We knew it could be addressed sometime down the line, just had find time for it. Here's the context and references to support this change compiler-research/CppInterOp#583 (comment)

@anutosh491
Copy link
Collaborator

Looks like the error occurs because emscripten-forge might be forcing the toolchain file, but we're building llvm-tblgen etc for the host correct ? This means we need to suppress it for 1st half and later we can restore the toolchain

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 21, 2025

Looks like the error occurs because emscripten-forge might be forcing the toolchain file, but we're building llvm-tblgen etc for the host correct ? This means we need to suppress it for 1st half and later we can restore the toolchain

By supress do you mean "unsetting the CMAKE_TOOLCHAIN_FILE" while building for host?

@anutosh491
Copy link
Collaborator

anutosh491 commented May 21, 2025

By supress do you mean "unsetting the CMAKE_TOOLCHAIN_FILE" while building for host?

Can possibly try that (passing empty string for the host rather than unsetting ?)

@anutosh491
Copy link
Collaborator

One small thing, I see this while building the llvm-tblgen

cmake -DLLVM_ENABLE_PROJECTS=clang -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Release ../llvm/ -DCMAKE_CROSSCOMPILING_EMULATOR=$BUILD_PREFIX/bin/node

I don't think we need the cross compiling emulator as we're building for host. Could you address this too ?

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 21, 2025

One small thing, I see this while building the llvm-tblgen

cmake -DLLVM_ENABLE_PROJECTS=clang -DLLVM_TARGETS_TO_BUILD=host -DCMAKE_BUILD_TYPE=Release ../llvm/ -DCMAKE_CROSSCOMPILING_EMULATOR=$BUILD_PREFIX/bin/node

I don't think we need the cross compiling emulator as we're building for host. Could you address this too ?

Done

@anutosh491
Copy link
Collaborator

Thanks for the changes, here hopefully everything build fine.

@kr-2003 kr-2003 marked this pull request as draft May 21, 2025 10:59
@kr-2003 kr-2003 changed the title Removed cross-compile patch from llvm-emscripten build [wip] Removed cross-compile patch from llvm-emscripten build May 21, 2025
@anutosh491
Copy link
Collaborator

Hey @kr-2003,

Through conda-forge (or through any llvm release) ,

We won't be having direct access to clang-tblgen. Here's the reason why the llvm-tblgen binary is provided but clang-tblgen is not !

https://discourse.llvm.org/t/clang-tblgen-not-installed/52997

Which means we need to build our own llvm-tblgen and clang-tblgen !

So we need to move along the lines of what you've done

  1. we deactivate the script
  2. build what we need for the build arch
  3. activate the script
  4. Build llvm

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 22, 2025

Hey @anutosh491,

Thanks.

I have pushed the changes. Made two new scripts (i.e. activats.sh and deactivate.sh) inside the recipe-directory.

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 22, 2025

Hey @DerThorsten,

Should each recipe have their own activation/deactivation scripts? Is it possible for us to use the scripts under /recipes/recipes/emscripten_emscripten-wasm32 in recipes/recipes_emscripten/llvm/build.sh.

@DerThorsten
Copy link
Contributor

Hey @DerThorsten,

Should each recipe have their own activation/deactivation scripts? Is it possible for us to use the scripts under /recipes/recipes/emscripten_emscripten-wasm32 in recipes/recipes_emscripten/llvm/build.sh.

I think in general, we want to avoid activation scripts when possible.
It might be possible to call the de-activation script by hand, but its a hack.

Also I am a bit unsure why we need to add new

Hey @kr-2003,

Through conda-forge (or through any llvm release) ,

We won't be having direct access to clang-tblgen. Here's the reason why the llvm-tblgen binary is provided but clang-tblgen is not !

https://discourse.llvm.org/t/clang-tblgen-not-installed/52997

Which means we need to build our own llvm-tblgen and clang-tblgen !

So we need to move along the lines of what you've done

  1. we deactivate the script
  2. build what we need for the build arch
  3. activate the script
  4. Build llvm

if you build for the "build-arch" then there is no emscripten whatsoever. So why would you need to do any disabling of activation scripts?

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 22, 2025

if you build for the "build-arch" then there is no emscripten whatsoever. So why would you need to do any disabling of activation scripts?

Yes, correct. My bad. I have moved llvm-tblgen and clang-tblgen to be built under llvm-tblgen pkg name in "build-arch". And then using this build with llvm-empscripten build.

@kr-2003 kr-2003 requested a review from anutosh491 May 23, 2025 03:24
@anutosh491 anutosh491 force-pushed the remove-c-comp-patch branch from d88f229 to 9de87f6 Compare May 23, 2025 04:08
Copy link
Collaborator

@anutosh491 anutosh491 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kr-2003 , this looks good.

I'll allow @DerThorsten to take the final call just to confirm everything's in place.

For some context

  1. llvm's emscripten build need llvm-tblgen/clang-tblgen being built for the build arch and then being shared through -DLLVM_TABLEGEN while building (discussion with Serge and other references can be found here compiler-research/CppInterOp#583 (comment))

  2. We were using a hacky patch to build llvm-tblgen at buildtime and then using it on the go. But would be much better without it.

  3. Hence this recipe should add a build arch recipe for llvm-tblgen and update the emscripten recipe for llvm to reference the location of the tablegen executable.

@anutosh491 anutosh491 marked this pull request as ready for review May 23, 2025 06:51
@anutosh491 anutosh491 force-pushed the remove-c-comp-patch branch from 9de87f6 to 2dad26e Compare May 26, 2025 11:20
@anutosh491 anutosh491 requested a review from DerThorsten May 26, 2025 11:20
@anutosh491 anutosh491 merged commit 77336e5 into emscripten-forge:main May 27, 2025
1 check passed
@anutosh491
Copy link
Collaborator

Hey @kr-2003,

Thanks for the changes here. Now that this has gone in, we need to test if this llvm build is working as per what we want.
As a follow up

  1. Fetch latest llvm locally once hosted (should take about 2 hours), build cppinterop on top of it and run the emscripten tests. All tests should pass as expected
  2. Once that is done, update cppinterop's recipe to use this llvm build (you would just need to increment the build number by 1)

Ping me once this is done.

@kr-2003
Copy link
Contributor Author

kr-2003 commented May 27, 2025

Hey @kr-2003,

Thanks for the changes here. Now that this has gone in, we need to test if this llvm build is working as per what we want. As a follow up

  1. Fetch latest llvm locally once hosted (should take about 2 hours), build cppinterop on top of it and run the emscripten tests. All tests should pass as expected
  2. Once that is done, update cppinterop's recipe to use this llvm build (you would just need to increment the build number by 1)

Ping me once this is done.

Its working 🚀 .
Screenshot 2025-05-27 at 4 55 21 PM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants